Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proposal] Add transaction management helper #314

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dmakushin
Copy link

@dmakushin dmakushin commented Nov 29, 2024

Suggestion is to add helper method similar to the one described at
https://bun.uptrace.dev/guide/transactions.html#runintx

@dmakushin dmakushin marked this pull request as draft November 29, 2024 12:31
@dmakushin
Copy link
Author

Hi @stephenafamo,

Could you please review this proposal?
If you find this idea viable, could you help me identify the correct place in the project to add this code?

@stephenafamo
Copy link
Owner

stephenafamo commented Nov 29, 2024

My first instinct is that this is not useful to Bob. While someone may decide to use a transaction wrapper, there is no need to bake this into Bob.

The reason is that every Bob method take a bob.Executor, which is similar to Bun's IDB interface.

  1. If you have a top level bob.DB, then you can start a transaction with tx, err := db.BeginTx(ctx, nil).
  2. If you have a top-level *sql.DB:
    var db *sql.DB
    tx, err := db.BeginTx(ctx, nil) // tx is *sql.Tx
    bobTxExec := bob.NewTx(tx)

You can then use this tx as your Bob executor, and then commit/rollback as you please (my preferred way is to create a new context and cancel with defer)

In your changes, I can rewrite your testOperationsWithDatabase to like this:

func testOperationsWithDatabase(ctx context.Context, db *sql.DB, value int32) error {
	ctx, cancel := context.WithCancel(ctx)
	defer cancel() // rollback transaction if not committed

	sqlTx, err := db.BeginTx(ctx, nil)
	if err != nil {
		return err
	}

	tx := bob.NewTx(sqlTx)

	q := psql.Update(
		um.Table("table"),
		um.Set(psql.Quote("field").EQ(psql.Arg(value))),
	)

	_, err = q.Exec(ctx, tx)
	if err != nil {
		return err
	}

	q2 := psql.Insert(
		im.Into("table2"),
		im.Values(psql.Quote("value").EQ(psql.Arg(value))),
	)

	_, err = q2.Exec(ctx, tx)
	if err != nil {
		return err
	}

	return tx.Commit()
}

@dmakushin
Copy link
Author

dmakushin commented Nov 29, 2024

@stephenafamo thank you for the quick response. I understand your opinion. I would like to have such a helper as part of bob because it is currently impossible to write a universal helper. The reason for this is that BeginTx returns a type specific to bob rather than sql.Tx. I understand why this was done, but it forces us to have a helper repository specifically for projects that will be written using bob. As a former user of bun, I am used to having such functionality included in the package itself.
Anyway, I will accept any decision.
Regarding the provided example, I am not sure that I understand how transaction will committed this way. Maybe I am not aware about some auto-commit mechanics which bob has?

@stephenafamo
Copy link
Owner

Regarding the provided example, I am not sure that I understand how transaction will committed this way. Maybe I am not aware about some auto-commit mechanics which bob has?

My bad 🤦🏾 . I missed calling tx.Commit() in my example. Changed the last line to return tx.Commit()

I would like to have such a helper as part of bob because it is currently impossible to write a universal helper. The reason for this is that BeginTx returns a type specific to bob rather than sql.Tx. I understand why this was done, but it forces us to have a helper repository specifically for projects that will be written using bob.

🤔 Alright, I see how this could be useful.
However, instead of placing this in a separate package, how about a method on bob.DB.

func (db bob.DB) InTx(ctx context.Context, fn TxFunc, opts ...TxOption) error {
    // ...
}

Would this work well for your usecase?

@dmakushin
Copy link
Author

I see the only benefit of a dedicated package is the ability to register additional callbacks like OnRollback and OnCommit, as provided in the example. However, in most cases, db.RunInTx will be sufficient.
I can prepare changes to add RunInTx as a DB method.

@dmakushin
Copy link
Author

@stephenafamo I updated PR, but I was unable to test these changes within my code, since the newest version has some breaking changes. After I regenerated the code I see many errors related to missing method Apply.
Additionally, conditions like the ones described here - https://bob.stephenafamo.com/docs/models/table#insert - aren't working anymore and now return a query instead of the inserted entity. Were these changes expected?

@stephenafamo
Copy link
Owner

Yes, there were a lot of changes in v0.29.0. Kindly take a look at CHANGELOG.md

Unfortunately, the docs are not updated yet 😔, also take a look at #310 for some details and examples.

@dmakushin
Copy link
Author

Thank you very much for the example. However, this has come as an unpleasant surprise. It will necessitate changes in several services on our side if we want to upgrade the version.

@stephenafamo
Copy link
Owner

I apologise for the breaking changes, it was not made lightly.

One of the main driving factors was making hooks robust. Unfortunately, it was very easy to write/use queries that bypassed hooks and I really want Bob to have very few footguns. If something can be done, it should work as expected.

@stephenafamo
Copy link
Owner

Hello @dmakushin 👋🏾

Any progress with this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants